Removing most node traversal usages #3009
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
olemartinorg
added
the
kind/other
added 12 commits
February 14, 2025 10:17
…odes() returned every node in pages in pageOrder, but nodeData included these implicitly hidden nodes. Removing generation of these nodes might break some use-cases, and I'll have to look into that more closely.
…This might have been a bug for a while!
…rect method. This failed hard _sometimes_ in the pdf.ts test, seemingly because the clearTimeout() that ran here failed hard when freezing clocks in cy.testPdf(). I have no idea what's going on, or why this suddenly happened now, but it seems like getting rid of more cruft fixes it.
…d to the backend before it continues to test the pdf.
…!== undefined check
…ng preselectedOptionIndex among others
19 tasks
added 5 commits
February 17, 2025 13:00
… them as 'isValid: false' instead, and use that info to not include them in validation selectors.
…eAttachmentDeletionInRepGroups.ts
…ably cheaper to run now
olemartinorg
changed the title
…roupTable.test.tsx
|
bjosttveit
reviewed
Feb 18, 2025
Comment on lines
+79
to
+83
| const componentOptions = NodesInternal.useShallowSelector((state) => | ||
| Object.values(state.nodeData).map((nodeData) => ({ | ||
| label: nodeData.layout.id, | ||
| value: `${nodeData.pageKey}|${nodeData.layout.id}`, | ||
| })), |
There was a problem hiding this comment.
A shallow selector will not do anything in this case no? You are returning a list, so the elements of the list are shallow-compared for equality, but you are creating new objects to populate the list every time so it will always fail the equality check on the first element
There was a problem hiding this comment.
Absolutely true! Sloppy mistake on my part. 🫣
bjosttveit
reviewed
Feb 18, 2025
Comment on lines
-33
to
+40
| NodesInternal.useEffectWhenReady(() => { | ||
| useEffect(() => { | ||
| if (shouldSelectOptionAutomatically) { | ||
| if (nodeStore.getState().readiness !== NodesReadiness.Ready) { | ||
| requestAnimationFrame(() => setForceUpdate((prev) => prev + 1)); | ||
| return; | ||
| } |
There was a problem hiding this comment.
That seems like a quite the machine gun of re-renders 🤯 The original useEffectWhenReady was a bit complex maybe, what was the issue here?
bjosttveit
reviewed
Feb 18, 2025
Comment on lines
+883
to
+885
| if (parent) { | ||
| return isHidden(state, 'node', parent?.layout.id, options); | ||
| } |
There was a problem hiding this comment.
Nitpick, parent cannot be null 😅
Suggested change
| if (parent) { | |
| return isHidden(state, 'node', parent?.layout.id, options); | |
| } | |
| if (parent) { | |
| return isHidden(state, 'node', parent.layout.id, options); | |
| } |
bjosttveit
reviewed
Feb 18, 2025
| @@ -308,6 +308,8 @@ describe('PDF', () => { | |||
| }); | |||
|
|
|||
| cy.goto('changename'); | |||
| cy.get(appFrontend.changeOfName.newFirstName).should('be.visible'); | |||
| cy.waitUntilSaved(); | |||
There was a problem hiding this comment.
Was this needed for the test to pass? 🤯 testPdf also waits for save before doing anything
bjosttveit
approved these changes
Feb 18, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
My plan has, for a while, been to remove the node traversal concept entirely. When that 's done, we can also remove the
LayoutPages,LayoutPageand even theLayoutNodeobject. At that point we can simplify the node hierarchy and refactor to remove the expression evaluation from the node generator.This is just one step along that road, removing some of the node traversal usages.
Related Issue(s)
Verification/QA
kind/*label to this PR for proper release notes grouping